Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: update deps #103

Closed
wants to merge 2 commits into from
Closed

chore: update deps #103

wants to merge 2 commits into from

Conversation

TheRogue76
Copy link
Contributor

@TheRogue76 TheRogue76 commented Jan 3, 2025 •
edited
Loading

Motivation

Bumping gradle, AGP and compile targets to match RN's core (Hence why i didn't go for gradle 8.12 or a higher AGP). Mostly just keeping things in sync, and i wanted to know if there was a specific reason this library was still targeting Java 8.

Summary

  1. Update gradle to 8.11.1
  2. Update com.android.library to 8.7.2 (why are we using this instead of specifically using AGP? I see 7621c81, but my android knowledge is not good enough to know why this was changed to the library itself.)
  3. Update the Java target to 17 to see if the CI would run. Looking at the action, it sets up with 17 anyway

Test Plan

CI. When running the tests locally, it complains about it targeting x86, but finding arm64 instead. Guessing it is either only made to run on the CI, or i missed an important step in the contribution guide
Screenshot 2025-01-03 at 03 42 35

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 3, 2025
@TheRogue76 TheRogue76 changed the title chore: bump gradle to 8.11.1, AGP to 8.7.2 and the Java version to 17 to match current master branch of RN chore: update deps Jan 3, 2025
cortinico
cortinico approved these changes Jan 3, 2025
View reviewed changes
gradle/wrapper/gradle-wrapper.properties Outdated Show resolved Hide resolved
@cortinico
Copy link
Contributor

and i wanted to know if there was a specific reason this library was still targeting Java 8

Mostly historical reasons.

Update com.android.library to 8.7.2 (why are we using this instead of specifically using AGP? I see 7621c81, but my android knowledge is not good enough to know why this was changed to the library itself.)

What do you mean? com.android.library is part of AGP. Not sure I follow your question here.

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@TheRogue76
Copy link
Contributor Author

TheRogue76 commented Jan 3, 2025 •
edited
Loading

Mostly historical reasons.

Makes sense

What do you mean? com.android.library is part of AGP. Not sure I follow your question here.

Misunderstanding on my part. I thought com.android.tools.build:gradle was the only way to define AGP and com.android.library was an umbrella for a bunch of other sub modules. All good.

I see the CI tests failed. error: cannot find symbol @Nullsafe(Nullsafe.Mode.LOCAL). Looking at the import (com.facebook.infer), i am not sure where this is supposed to be coming in from. Is it supposed to be auto generated onceassembleDebug is run?

@TheRogue76
Copy link
Contributor Author

Error seems to have started with this: cdd56a7

@cortinico
Copy link
Contributor

Error seems to have started with this: cdd56a7

I've sent this one #104 which should fix the build issue. Once that is in, we can rebase yours on top of main

@facebook-github-bot
Copy link
Contributor

@TheRogue76 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@cortinico merged this pull request in d72c2f3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cortinico cortinico cortinico approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TheRogue76 @cortinico @facebook-github-bot